Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address issues in auto-generated CDDL specification #4553

Merged
merged 16 commits into from
Sep 29, 2024
Merged

Address issues in auto-generated CDDL specification #4553

merged 16 commits into from
Sep 29, 2024

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented Aug 16, 2024

Description

This addresses issue #4462

  • Moves some core type CDDL definitions to cardano-ledger-core
  • Defines Shelley CDDL types through Huddle
  • Adds huddle-based testing in Shelley
  • Moves the Shelley CDDL to being auto-generated via the Huddle def
  • Updates Conway to use some definitions from Shelley where they were unchanged
  • Updates the gen-cddl.sh script to check Shelley definitions

One unexpected change: the huddle definition for protocol_param_updates has a size limit on the max block header size. This is consistent with the Haskell serialisers and data structure.

In addition, we add a couple of extra commits solving two issues:

Note that this PR now incorporates the contents of #4552

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
    New section is never added with the code changes. (See RELEASING.md)
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated.
    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@nc6 nc6 requested a review from lehins August 16, 2024 11:14
@nc6
Copy link
Contributor Author

nc6 commented Aug 19, 2024

Note that this is now out-of-date with #4552 - I'll update after that one is merged.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nc6 Please fix the formatting and I'll do the proper then.
Sorry, I just can't look at the trailing commas they hurting my eyes.

eras/conway/impl/CHANGELOG.md Outdated Show resolved Hide resolved
eras/conway/impl/cddl-files/conway.cddl Outdated Show resolved Hide resolved
@mpizenberg
Copy link

In addition to documentation comments, is it possible to regroup definitions please. For example, currently the conway certificates definitions are all over the place. It seems they are sorted alphabetically, which isn’t useful.

@teodanciu
Copy link
Contributor

Rebased on master, to remove the master-merge commit

nc6 and others added 12 commits September 28, 2024 22:06
Now that the CDDL is defined with Cuddle, we can
begin to properly modularise it. As a first step,
we move the common crypto and utility types into
the core package.
- Switch existing tests to using the generated CDDL
- Add additional Huddle based tests for Shelley types
- Add a tool to regenerate the Shelley CDDL from Huddle
- Move additional core types to the core CDDL
- Make a few fixes in the Shelley Huddle spec

There is one unusual thing here: the size bound on
the max block header size in the protocol param
update. This does not reflect the original CDDL,
but it is consistent with the FromCBOR instance
and the underlying data type in PParams. I can
only assume that the CDDL generator wasn't
exploring the whole range and thus never found
this error.
Now that Shelley is defined using Huddle, we can
rely on the relevant parts from Conway.
We use 'here' QuasiQuotes to make handling
multiline comments somewhat less painful.
This addresses issue #4535. The first rule in a
CDDL file is taken to be the root element.
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CDDL is looking better and better 😀

@lehins lehins merged commit 1071d0f into master Sep 29, 2024
151 of 155 checks passed
@lehins lehins deleted the nc/4522 branch September 29, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants